Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dockerfile: allow multiple values for ARG #1692

Merged
merged 1 commit into from
Sep 24, 2020

Conversation

tonistiigi
Copy link
Member

ENV accepts multiple definitions with a single command, no reason why ARG should behave any differently.

Signed-off-by: Tonis Tiigi tonistiigi@gmail.com

@thaJeztah
Copy link
Member

So, if I recall correctly, we didn't do this, because ARG and ENV are very similar, but (unlike ARG) ENV does not enforce = to set a value (similar discussion in moby/moby#17344).

Because of that, the following would be very confusing as they have completely different meanings:

# declare three ARGs: ONE, TWO, and THREE
ARG ONE TWO THREE

# define one ENV (ONE) with value "TWO THREE"
ENV ONE TWO THREE

So, the ARG syntax, by enforcing = to set a value, is more consistent and flexible as it would allow combinations of "declaring" args with and without a value in a single ARG:

ARG ONE=hello

FROM busybox AS stage-foo

# declare three ARGs:
ARG ONE TWO= THREE=world

In the example above;

  • arg ONE doesn't have a value set, and in this case inherits its value from the global ARG ONE
  • arg TWO is explicitly set to an empty string as default value (quotes are optional, so equivalent to ARG TWO="")
  • arg THREE has "world" as default value

Env vars did get some validation to try to prevent some accidental mistakes;

ENV ONE=one TWO two THREE three

Syntax error - can't find = in "TWO". Must be of the form: name=value

But will only produce an error if the first env-var uses an =, so the ARG example above will silently "succeed":

ENV ONE TWO= THREE=world

And produce a single env-var (ONE) with value "TWO= THREE=world"

So not sure what to do with this. I realize ARG and ENV are separate things, but they are so similar that I anticipate users to have them work the same, and trip over the differences.

My ideal would be to use the same syntax for ENV, but changing would be a breaking change, and likely will hit many existing Dockerfiles.

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

leaving a "request changes" review, as this may need some discussion (see my comment)

@tonistiigi
Copy link
Member Author

If you are annoyed about the ambiguity of ENV we should start to phase out ENV key value. Not to remove it yet but deprecate, remove docs, make sure linters/language-servers warn against it etc. Our own Dockerfiles already don't use it. ARG should not use this, first it is just confusing, and ARG inheritance is an important feature.

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After discussing above, and docker/cli#2741, docker/cli#2743 to reduce confusion, I think it's ok to support this

LGTM

@thaJeztah
Copy link
Member

We should probably update the legacy builder as well to support the format?

@tiborvass tiborvass merged commit 718f86c into moby:master Sep 24, 2020
@cpuguy83
Copy link
Member

I would argue that allowing ARG key value is nicer than ARG key=value key2=value2 or even ARG key1 key2... and possibly less inconsistent.

The whole reason ENV foo=bar exists is because we wanted to be able to ENV foo=bar baz=quux in a single layer... which is no longer a problem since these don't get layers.

@tonistiigi
Copy link
Member Author

I would argue that allowing ARG key value

ARG key value is just impossible syntax as ARG key= and ARG key have different meanings.

and possibly less inconsistent.

Why? The whole Dockerfile language is modeled after shell, and KEY=value [KEY2=value] is what you do in shell. Also consistent with RUN KEY=value command.

wanted to be able to ENV foo=bar baz=quux in a single layer

Where I've seen it, it has always looked like a logical grouping of related values. That is also why having same for ARG is nice. So you can do ARG TARGETOS TARGETARCH TARGETVARIANT in a single line.

bartmoorman added a commit to bartmoorman/Docker-Minecraft that referenced this pull request Dec 23, 2020
zachaller added a commit to zachaller/argo-cd that referenced this pull request Mar 23, 2022
Podman and older versions of docker do not support multiple args
on a single line. It was recently added to docker in this commit
moby/buildkit#1692 and podman still dose not have support
for it.

Signed-off-by: zachaller <zachaller@hotmail.com>
crenshaw-dev pushed a commit to argoproj/argo-cd that referenced this pull request Mar 23, 2022
Podman and older versions of docker do not support multiple args
on a single line. It was recently added to docker in this commit
moby/buildkit#1692 and podman still dose not have support
for it.

Signed-off-by: zachaller <zachaller@hotmail.com>
wojtekidd pushed a commit to wojtekidd/argo-cd that referenced this pull request Apr 25, 2022
Podman and older versions of docker do not support multiple args
on a single line. It was recently added to docker in this commit
moby/buildkit#1692 and podman still dose not have support
for it.

Signed-off-by: zachaller <zachaller@hotmail.com>
Signed-off-by: wojtekidd <wojtek.cichon@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants